import tarfile #62

ghost · 2019-07-30T17:17:39Z

Signed-off-by: laven-s laven.s@ibm.com

makandre · 2019-07-30T17:20:43Z

src/initialize/src/controllers/tarController.ts

+    const fileName = name + '.tar.gz';
+    let file = fs.createWriteStream(fileName);
+
+    await new Promise((resolve, reject) => {


Why don't we return the Promise here? It will be awaited by the caller

makandre · 2019-07-30T17:21:11Z

src/initialize/src/controllers/tarController.ts

+        })
+    })
+    .catch(error => {
+        throw new Error('Extracting tar file failed');


I think we should reject the error here?

makandre · 2019-07-30T19:08:38Z

src/initialize/src/controllers/tarController.ts

+const { exec } = require('child_process');
+import request from 'request';
+
+export async function importProjectFromTar(tarFile: string, name: string, dest: string): Promise<void> {


If we get rid of the await below this doesn't need to be a async function

makandre · 2019-08-01T12:52:27Z

@laven26 How are the changes coming along?

makandre · 2019-08-01T16:56:31Z

src/initialize/src/controllers/tarController.ts

+              fs.removeSync(fileName);
+          });
+
+        resolve();


Just noticed something... this actually needs to go into exec callback above. And removeSync should have try/catch

if (err) return reject(err); try { fs.removeSync(fileName); } catch (err) { // probably just log this here. } resolve();

makandre · 2019-08-01T16:57:07Z

src/initialize/src/controllers/tarController.ts

+        resolve();
+
+    })
+    .on('error', (error) => {


A more concise syntax here can be `.on('error', reject);

makandre · 2019-08-01T18:24:15Z

src/initialize/src/classes/ProjectInitializer.ts

@@ -47,6 +48,16 @@ export default class ProjectInitializer {
    return this.initializeExistingProject();
  }

+  async initializeProjectFromTar(
+    tarFile: string
+    ) {


missed return type here

makandre · 2019-08-01T18:53:05Z

LGTM

sghung · 2019-08-01T19:30:02Z

@rajivnathan can you help review this? Thank you

sghung · 2019-08-06T15:58:18Z

@hhellyer , can you help review this? Thank you

hhellyer

I think this needs a rebase to bring it up to date as well as we've actually added the request module in the meantime. If you wanted to pull out the asyncHttpDownload or the execAsync function from #100 to a utility file that might be good but not essential.

hhellyer · 2019-08-07T15:30:23Z

src/initialize/src/main.ts

@@ -42,7 +42,9 @@ export async function initializeProject(projectName: string, projectDirectory: s
  const projectInitializer = new ProjectInitializer(projectName, projectDirectory);

  try {
-    if (isGitCloneRequired(projectName, gitRepository)) {
+    if (gitRepository.endsWith("tar.gz")) {


I'm not that comfortable with overloading GIT_REPO to also mean "any url to a tar.gz". Can we just use a different env var and check if that was set or if GIT_REPO was set?

hhellyer · 2019-08-07T15:34:09Z

src/initialize/src/controllers/tarController.ts

+            resolve();
+          });
+    })
+    .on('error', reject);


Does this remove the empty or partial file if the download fails? (e.g. on a bad url or github being down)

hhellyer · 2019-08-07T15:42:36Z

src/initialize/src/controllers/tarController.ts

+export function importProjectFromTar(tarFile: string, name: string, dest: string): Promise<void> {
+
+    const fileName = name + '.tar.gz';
+    const file = fs.createWriteStream(fileName);


This should probably be an absolute filename, otherwise it will just be created in the current working directory (and I'm not sure exactly where that will be in all cases).

Please use /tmp not /app. /app exists in the initialiser pod but the initialiser code is run directly as an npm when we are in che and /app doesn't exist there.
(Also /tmp is a better location for temporary files.)

Signed-off-by: laven-s <laven.s@ibm.com>

makandre · 2019-08-08T20:14:02Z

src/pfe/portal/modules/projectInitializer.js

+        `PROJ_NAME=${projectName}`,
+        `GIT_REPO=${gitInfo.repo}`,
+      );
+    }


The gitInfo.branch check below can go into this else block right? Since it would be applicable only for non-tarball scenario

makandre · 2019-08-08T20:15:15Z

src/pfe/portal/modules/projectInitializer.js

-      `PROJ_NAME=${projectName}`,
-      `GIT_REPO=${gitInfo.repo}`,
-    );
+    if (gitInfo.repo.endsWith("tar.gz")) {


A minor nit: while you're making changes, could you make this check for .tar.gz? (add dot in the front)

Signed-off-by: laven-s <laven.s@ibm.com>

makandre · 2019-08-09T17:07:27Z

src/initialize/src/controllers/tarController.ts

+            resolve();
+          });
+    })
+    .on('error', () => {


I believe there's an err passed here you can then pass it to reject

Signed-off-by: laven-s <laven.s@ibm.com>

sghung · 2019-08-13T15:30:49Z

Thanks for the reviews Andrew and Howard

makandre reviewed Jul 30, 2019

View reviewed changes

makandre mentioned this pull request Jul 30, 2019

Dynamically return template repositories to Codewind kabanero-io/appsodyExtension#28

Closed

makandre reviewed Jul 30, 2019

View reviewed changes

makandre reviewed Aug 1, 2019

View reviewed changes

makandre mentioned this pull request Aug 1, 2019

Adding a devfile registry index appsody/stacks#131

Closed

4 tasks

sghung assigned ghost Aug 1, 2019

sghung requested a review from rajivnathan August 1, 2019 19:29

sghung removed the request for review from rajivnathan August 6, 2019 15:57

sghung requested a review from hhellyer August 6, 2019 20:43

hhellyer suggested changes Aug 7, 2019

View reviewed changes

hhellyer reviewed Aug 7, 2019

View reviewed changes

laven-s added 3 commits August 8, 2019 12:33

import tarfile support

5e34481

Signed-off-by: laven-s <laven.s@ibm.com>

add env variable for tar file

d781518

Signed-off-by: laven-s <laven.s@ibm.com>

fix return

730e6f8

Signed-off-by: laven-s <laven.s@ibm.com>

makandre reviewed Aug 8, 2019

View reviewed changes

fix disabled projects

53ec54f

Signed-off-by: laven-s <laven.s@ibm.com>

makandre reviewed Aug 9, 2019

View reviewed changes

laven-s added 2 commits August 9, 2019 13:16

remove file if download fails

d99c128

Signed-off-by: laven-s <laven.s@ibm.com>

change temp file path

4350f26

Signed-off-by: laven-s <laven.s@ibm.com>

makandre approved these changes Aug 12, 2019

View reviewed changes

hhellyer approved these changes Aug 13, 2019

View reviewed changes

sghung merged commit 1581cbd into eclipse-archived:master Aug 13, 2019

rwalle61 mentioned this pull request Aug 14, 2019

add ability to download from tar.gz; add tests eclipse-archived/codewind-installer#75

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

import tarfile #62

import tarfile #62

ghost commented Jul 30, 2019

makandre Jul 30, 2019

makandre Jul 30, 2019

makandre Jul 30, 2019

makandre commented Aug 1, 2019

makandre Aug 1, 2019 •

edited

Loading

makandre Aug 1, 2019

makandre Aug 1, 2019

makandre commented Aug 1, 2019

sghung commented Aug 1, 2019

sghung commented Aug 6, 2019 •

edited

Loading

hhellyer left a comment

hhellyer Aug 7, 2019

hhellyer Aug 7, 2019

hhellyer Aug 7, 2019

hhellyer Aug 12, 2019 •

edited

Loading

makandre Aug 8, 2019 •

edited

Loading

makandre Aug 8, 2019

makandre Aug 9, 2019

sghung commented Aug 13, 2019

import tarfile #62

import tarfile #62

Conversation

ghost commented Jul 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makandre commented Aug 1, 2019

makandre Aug 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makandre commented Aug 1, 2019

sghung commented Aug 1, 2019

sghung commented Aug 6, 2019 • edited Loading

hhellyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhellyer Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

makandre Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sghung commented Aug 13, 2019

makandre Aug 1, 2019 •

edited

Loading

sghung commented Aug 6, 2019 •

edited

Loading

hhellyer Aug 12, 2019 •

edited

Loading

makandre Aug 8, 2019 •

edited

Loading